Thanks for opening this pull request!

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (b50790a) to head (fdba10f).
Report is 47 commits behind head on alpha.

@@            Coverage Diff             @@
##             alpha     #2117    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           61        64     +3     
  Lines         6186      6398   +212     
  Branches      1499      1543    +44     
==========================================
+ Hits          6186      6398   +212     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Can you check code coverage? Looks like you are missing a test.

Another alternative solution that might be preferred is to put a similar fix into ParseObject._getSaveJSON() instead? That means the Ops array stays the same (which is actually what the tests should check I'll fix that, that's in the end what the server cares about). But the data is merged in the saveJSON only.

@mortenmo Good Work! Can you add an integration test to make sure it's not crashing?

@mortenmo Good Work! Can you add an integration test to make sure it's not crashing?

Yes. Took a subset of related testcases and made versions that don't save between the object creation and setting/unsetting making sure the results are the same. Just had to figure out how to run them :)

@dplewis type check was failing I assume from the changing CoreManager to ts. I put in a fix here but have to back that out if you fixed it somewhere else as well.

Yeah sorry for the confusion. Can you revert changes unrelated to the issue? This PR looks good to me. I just to test it myself. Luckily I’ve been looking into dot notation and the internals recently so it shouldn’t take too long

@mortenmo Can you rebase from alpha? ParseObject was converted to typescript and is causing conflicts in this PR.

Made some git mistakes that my git skillz couldn't get me out of, but should be good now.

Isn't there more to this issue? In #1798 it's described that it causes an internal server error, which sounds like Parse Server needs a patch as well, to that doesn't crash on malformed request?

Isn't there more to this issue? In #1798 it's described that it causes an internal server error, which sounds like Parse Server needs a patch as well, to that doesn't crash on malformed request?

@mtrezza I don't think so. The reason the server "barfed" on the Ops is that Mongo (maybe also Postgres?) doesn't support chained updates when you changed the "parent". When you change/set a property (eg object) to an object and then do a set object.foo to something in the same save, Mongo threw an error. This PR merges those on the client before sending to server (whether it is set or unset), so the server doesn't get those Op commands anymore.

It is unclear to me if the server should support an operation to set object={ ... } and set object.name='foo' in the same save or if the clients should avoid it. It is only a problem if you set a parent and then a "child" property. If you think the server should, then yes there should be another issue opened on the server. But the JS SDK shouldn't trigger that problem anymore, but others might?

I mean it should not be possible to cause the server to crash with internal server error. Even if the Parse JS SDK doesn't send this anymore, it would still be possible to send this to Parse Server via the REST API and cause the crash, right?

When I tried it manually, server does respond with a 500, but to be clear doesn't completely crash. But yes, it is possible to send through and at a minimum a validation server side with a 400 response is probably better.

I believe a 500 error may actually be a process crash.